The latest Buf updates on your PR. Results from workflow CI / buf-check (pull_request).

Claude finished @julienrbrt's task in 2m 17s —— View job

Code Review: feat(sequencer): catchup from base

• Gathering context and reading changed files
• Reviewing sequencer catch-up logic
• Reviewing syncer DAHeight changes
• Reviewing DA client/tracing changes
• Reviewing tests
• Writing final review

Summary

This PR adds a sequencer catch-up mechanism to handle restarts after extended downtime, where the sequencer replays missed DA epochs using only forced-inclusion transactions. The overall design is sound, and the incremental DAHeight advancement in the syncer is well-reasoned. Below are the findings, from most to least critical.

Critical

1. currentDAEndTime is not protected by the atomic types — potential data race

pkg/sequencers/single/sequencer.go:71

currentDAEndTime time.Time

catchUpState is correctly stored as atomic.Int32, but currentDAEndTime (a time.Time struct — 3 words on 64-bit) is written in fetchNextDAEpoch and read concurrently in GetNextBatch. time.Time is not atomically readable, so this is a data race on any platform that splits multi-word reads. The public isCatchingUp() (previously named IsCatchingUp in an earlier revision) was already flagged by Gemini; while it's now unexported, the currentDAEndTime race remains.

Fix: protect with a sync.RWMutex, or store a *atomic.Pointer[time.Time].

Fix this →

High

2. Timestamp jitter scheme assumes 1 ms/tx — could still produce duplicate timestamps with 0-tx batches

pkg/sequencers/single/sequencer.go:373–377

remainingForcedTxs = uint64(len(c.cachedForcedInclusionTxs)) - c.checkpoint.TxIndex
timestamp = daEndTime.Add(-time.Duration(remainingForcedTxs) * time.Millisecond)

The jitter is computed as -(remaining txs * 1ms). This is a reasonable monotonicity trick, but breaks in at least two cases:

• Empty epoch + 0-tx batch: When cachedForcedInclusionTxs is empty (an epoch with no forced txs), remainingForcedTxs is 0, so the timestamp equals daEndTime. If the previous epoch's last block also had remainingForcedTxs == 0, both blocks share the same timestamp. The test TestSequencer_CatchUp_MonotonicTimestamps_EmptyEpoch does cover this case and verifies it works, but only because consecutive epoch timestamps differ by 15 seconds. If two back-to-back empty epochs have the same DA end timestamp (unlikely but possible with clock rounding), timestamps would collide.

• Forced tx filtered by FilterRemove: A tx counted by len(cachedForcedInclusionTxs) may be FilterRemoved, advancing TxIndex without producing a block. The remaining count then skips values, potentially repeating a timestamp from a prior batch.

The test suite covers the happy path well but lacks coverage for the FilterRemove path during catch-up. This is partially why patch coverage for sequencer.go shows 4 missing lines and 2 partials.

3. updateCatchUpState makes a synchronous RPC call (GetLatestDAHeight) inside the hot block-production path

pkg/sequencers/single/sequencer.go:531–589

updateCatchUpState is called from fetchNextDAEpoch, which is called from GetNextBatch. On first invocation it issues GetLatestDAHeight, a potentially slow network call. If the DA layer is slow or unreachable, block production blocks. The design comment says "runs exactly once", which limits the impact in practice, but there's no timeout on the call other than the daClient's defaultTimeout (60 s by default). If this call hangs, block production stalls for a full minute.

Consider calling updateCatchUpState asynchronously during NewSequencer initialization, or accepting a context with a shorter deadline.

Medium

4. syncer.go — "large gap" heuristic is too broad; may misfire on legitimate fast DA layers

block/internal/syncing/syncer.go:784

if epochSize > 0 && gap > epochSize {

The condition gap > epochSize treats any gap larger than one epoch as catch-up blocks. But this logic runs on all syncing nodes, not just the sequencer itself. A legitimate fast-syncing full node (e.g., just started from genesis with a high DA start height) will also produce large gaps and incorrectly trigger epoch-by-epoch advancement. The consequence is that the forced-inclusion verifier will check old epochs for blocks that were actually produced at the current DA height, potentially causing false negatives in malicious-proposer detection.

Consider adding a distinguishing signal (e.g., a custom field in DAHeightEvent or checking event.Source) to limit this heuristic to blocks produced during sequencer catch-up only.

5. Magic constant 1ms jitter is not defined

pkg/sequencers/single/sequencer.go:376

timestamp = daEndTime.Add(-time.Duration(remainingForcedTxs) * time.Millisecond)

The time.Millisecond multiplier is undocumented. A named constant (const catchUpTimestampJitter = time.Millisecond) with a comment explaining the choice would help future readers, and would make it easier to tune if needed. Gemini flagged a similar issue for the 30 * time.Second constant; the 1ms jitter is equally worth naming.

6. catchUpState race-free but ToCAS (compare-and-swap) pattern incomplete

pkg/sequencers/single/sequencer.go:531–537

func (c *Sequencer) updateCatchUpState(ctx context.Context) {
    if c.catchUpState.Load() != catchUpUnchecked {
        return
    }
    // Optimistically mark as done; ...
    c.catchUpState.Store(catchUpDone)

The load-then-store is not atomic (two separate operations). If two goroutines call updateCatchUpState concurrently, both could pass the Load() check and then both execute the body, resulting in two GetLatestDAHeight calls and a race on the state transition. Use CompareAndSwap(catchUpUnchecked, catchUpDone) to make this atomic:

if !c.catchUpState.CompareAndSwap(catchUpUnchecked, catchUpDone) {
    return
}

Fix this →

7. da/client.go timestamp fallback uses time.Now() with a TODO comment

block/internal/da/client.go:228

blockTime = time.Now()
// TODO: we should retry fetching the timestamp. Current time may mess block time consistency for based sequencers.

This pre-existing TODO becomes more urgent in this PR: the catch-up mechanism depends on accurate DA block timestamps for the jitter scheme. If getBlockTimestamp fails during catch-up and falls back to time.Now(), block timestamps will be wrong (current time instead of historical time), breaking the invariant the PR tries to enforce. At minimum, the comment should note that catch-up mode in the sequencer is also affected.

Low

8. isCatchingUp() is unexported but catchUpState is read directly in several places

pkg/sequencers/single/sequencer.go:358, 369, 451, 506

Multiple places call c.catchUpState.Load() == catchUpInProgress inline rather than using the isCatchingUp() helper. This is inconsistent and increases the chance of a bug if the state machine grows. Centralizing all state checks through isCatchingUp() would be cleaner.

9. Test logger inconsistency

pkg/sequencers/single/sequencer_test.go:1245, 1315

logger := zerolog.New(zerolog.NewTestWriter(t))

Some catch-up tests use zerolog.NewTestWriter(t) (good), others use zerolog.New(zerolog.NewConsoleWriter()) (lines 1309, 1457, 1676 as flagged by Gemini). This produces noise in CI. All tests should use zerolog.Nop() or zerolog.NewTestWriter(t).

10. tracing.go — missing RetrieveForcedInclusion tracing wrapper

block/internal/da/tracing.go does not wrap the RetrieveForcedInclusion method even though it was added to client.go. This means forced-inclusion retrievals will not appear in distributed traces. The tracing_test.go's mockFullClient implements it as a no-op stub but no test covers the tracing path.

Architecture Notes

• The incremental DAHeight advancement in TrySyncNextBlock (syncer.go:784–805) is well-commented and technically correct for the sequencer-catch-up case. The risk identified in point 4 above is worth discussing before merge.
• The catch-up lifecycle state machine (unchecked → inProgress | done) is clean and the use of atomic.Int32 for catchUpState is correct.
• The fetchNextDAEpoch design (one-time check + ErrHeightFromFuture exit) is pragmatic and avoids polling overhead.
• Test coverage for the new happy paths is good. The main gaps are: the FilterRemove-during-catch-up path, the concurrent access path, and the fast-syncing-node false-positive in syncer.go.

Summary of Changes

Hello @julienrbrt, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request implements a critical "catch-up" feature for the sequencer, designed to enhance its resilience and consistency following periods of inactivity or downtime. By intelligently replaying missed Data Availability (DA) epochs, the sequencer can resynchronize its state, ensuring that all forced inclusion transactions are processed in the correct order and with accurate timestamps, thereby maintaining a consistent view of the blockchain's history, even when operating in a decentralized environment. This mechanism allows the sequencer to seamlessly transition back to normal operation without compromising data integrity.

Highlights

• Sequencer Catch-up Mechanism: Introduced a new mechanism allowing the sequencer to "catch up" on missed Data Availability (DA) epochs after extended downtime.
• Forced Inclusion Priority: During catch-up, the sequencer temporarily processes only forced inclusion transactions, ensuring consistency with base sequencing nodes and temporarily skipping mempool transactions.
• DA Height Synchronization: Modified syncer.go to incrementally advance the DAHeight by one epoch during catch-up, preventing issues with forced inclusion transaction verification.
• Timestamp Alignment: Block timestamps generated during catch-up are now aligned with the DA epoch's end timestamp for historical accuracy.
• Comprehensive Testing: Added extensive unit tests covering various catch-up scenarios, including detection, mempool skipping, timestamp usage, exit conditions, and multi-epoch replay.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Codecov Report

❌ Patch coverage is 64.86486% with 39 lines in your changes missing coverage. Please review.
✅ Project coverage is 61.24%. Comparing base (cf5565e) to head (d7b8b2c).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##             main    #3057      +/-   ##
==========================================
+ Coverage   61.16%   61.24%   +0.08%     
==========================================
  Files         113      113              
  Lines       11444    11548     +104     
==========================================
+ Hits         7000     7073      +73     
- Misses       3655     3686      +31     
  Partials      789      789              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.